Across MI by RaspberryEmma · Pull Request #2 · opensafely/post-covid-cvd-missingness-methods

RaspberryEmma · 2026-06-23T14:02:46Z

No description provided.

dates are correctly excluded by MICE without needing to have them removed from the dataframe due to the shift to defensive coding style, awkward workaround has been removed.

pmadleydowd · 2026-06-25T09:43:10Z

Emma - really???

I don't think it's as barmy as it looks. If tomorrow Kate said to change the number of imputed datasets from 10 to 20 for example, then we could change it in just one place instead of changing it manually across every other file.

The weights function is another instance where it would need to be copy-pasted, but putting it here ensures consistency.

see also the rule of three (https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming))

ok - i retract my statement - good job thinking ahead

pmadleydowd · 2026-06-25T09:54:26Z

updates to add function instead of absolute number

addition of weights to each model to account for number of imputations.

pmadleydowd · 2026-06-25T09:54:39Z

updates to add function instead of absolute number

addition of weights to models

pmadleydowd · 2026-06-25T09:54:46Z

updates to add function instead of absolute number

additon of weights to models

pmadleydowd · 2026-06-25T09:55:04Z

updates to add function instead of absolute number

pmadleydowd · 2026-06-25T10:25:14Z

+  data   = logistic_df,
+  weights = generate_weights(
+    sample_size = nrow(model_input_df),
+    num_imps    = 10


should this line take the new function for imputations as its argument instead so that you can update any weights at the same time as the number of imputations

i would place this in the generate_weights function as the default instead of null

Hiya, sorry I think this might be an after effect of going one commit at a time, this line is updated on my end / the version as of yesterday's last commit. Good call r.e. having that as a default though, I'll get that actioned.

actioned change, see below

pmadleydowd · 2026-06-25T10:40:23Z

+# Apply multiple imputation for sahhs outcome
+imp_sahhs <- mice::mice(
+  data       = df_no_vax_dates_sahhs,
+  m          = 10, # hard-coded to match below


may have been updated later but check that m has been updated to use the new function here also

This is updated in a later commit yes.

pmadleydowd · 2026-06-25T10:41:46Z

+
+# Re-assign unique identifiers to imputed dataset for outcome  ---
+print("Re-assign unique identifiers to imputed dataset for outcome ")
+


the below wont work if we update the number of imputations - I wouldn't bother changing at this point as Im fairly sure we will stick with 10 imputations - but need to be aware of this if we do - appreciate this suggestion is not in line with defensive coding

i still dont understand why this is really needed

The cox_ipw action requires unique patient id per row as part of data preparation (see: https://github.com/opensafely-actions/cox-ipw/blob/726915c1b57a1df1dac8e7fe37af782b876f3380/analysis/cox-ipw.R#L512)

Good point that the patient_id reassignment is still stuck in the old way (hard coded to need 10, so still an improvement overall but this part isn't dynamic). This means we would strictly need to change the number of imputed datasets in two places (in utility.R and here where they're assigned) to make it work.

There is a part of me that knows that making this dynamic would be straightforward (generate vector of variable size using recursive pattern), I've just not done that (really trying to speed up on my own end)

actioned change, see below

RaspberryEmma · 2026-06-25T11:13:44Z

Hiya, I've implemented the new weights for the cox_ipw action (see most recent commit). I'm currently actioning your suggested change (having generate weights call the number of imputations function as a default)

RaspberryEmma · 2026-06-25T12:26:20Z

Hiya, I've implemented making the new patient ID script fully dynamic

made the num_imps parameter optional, with default value taken from get number of imputed datasets function

RaspberryEmma · 2026-06-25T12:37:55Z

Hiya, implemented the refactor of the generate_weights function suggested above (good call btw, it's much tidier now), I believe that makes everything and the code should be good to merge and run unless there's any further feedback

RaspberryEmma · 2026-06-29T12:41:09Z

Hi Paul, I've addressed all comments from the code review you provided Thursday evening, those being:

table1 output only running for the version of the data where the Nelson-Aalen estimator is calculated for ami
The codelists update (addressed verbally in last week's meeting)

The other PR is an erroneous copy of this one (apologies) and has been deleted. The provided feedback all addressed, am I now good to merge and run?

Updated the convert_terms_to_vars function to use a predefined list of variable names instead of relying on term splitting.

unable to do SEs, need to talk over

currently many are zero, to be discussed.

RaspberryEmma added 3 commits June 19, 2026 14:06

partial implementation

f1f7dcc

complete across MI implementation, posthoc removed

2eafbe4

Delete analysis/post_hoc_vars directory

fb7a73f

RaspberryEmma requested a review from pmadleydowd June 23, 2026 14:02

RaspberryEmma added 4 commits June 23, 2026 16:19

no longer remove dates from dataframe

e4505c2

dates are correctly excluded by MICE without needing to have them removed from the dataframe due to the shift to defensive coding style, awkward workaround has been removed.

Added 1/M weights to all models except cox_ipw

c496389

hotfix dataframes

c3df6b8

refactor: removed magic number for imp datasets

468b6ef

pmadleydowd reviewed Jun 25, 2026

View reviewed changes

Comment thread analysis/apply_across_MI/apply_across_MI.R

pmadleydowd Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updates to add function instead of absolute number

pmadleydowd reviewed Jun 25, 2026

View reviewed changes

custom weights for cox_ipw reusable action

15cb61e

fully dynamic number of imputed datasets

0ea453b

refactored generate_weights function

706ec19

made the num_imps parameter optional, with default value taken from get number of imputed datasets function

table1, table2 output changes

4d01292

RaspberryEmma mentioned this pull request Jun 29, 2026

Across MI implementation completed opensafely/post-covid-cvd-methods#25

Closed

RaspberryEmma added 5 commits July 6, 2026 13:13

hotfixes: bmi handling and gsub bug

d3aa1c3

convert_terms_to_vars fix

f734c60

Updated the convert_terms_to_vars function to use a predefined list of variable names instead of relying on term splitting.

new action to generate Nelson-Aalen plots

aa480ba

unable to do SEs, need to talk over

table of missingness values

b75042c

currently many are zero, to be discussed.

robust SE in cox models

24a8952

nelson aalen SEs and updated plots

4846359


		# Re-assign unique identifiers to imputed dataset for outcome ---
		print("Re-assign unique identifiers to imputed dataset for outcome ")

Uh oh!

Conversation

RaspberryEmma commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmadleydowd Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaspberryEmma commented Jun 25, 2026

Uh oh!

RaspberryEmma commented Jun 25, 2026

Uh oh!

RaspberryEmma commented Jun 25, 2026

Uh oh!

RaspberryEmma commented Jun 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pmadleydowd Jun 25, 2026 •

edited

Loading